-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make SampleCurve
/UnevenSampleCurve
succeed at reflection
#15493
Make SampleCurve
/UnevenSampleCurve
succeed at reflection
#15493
Conversation
…cit interpolation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a reflection perspective this looks good to me!
|
||
/// A curve that is defined by explicit neighbor interpolation over a set of evenly-spaced samples. | ||
#[derive(Clone)] | ||
#[cfg_attr(feature = "serialize", derive(serde::Serialize, serde::Deserialize))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we #[reflect(Serialize, Deserialize)]
? Same goes for the other types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into issues with that with the generics, so I'd prefer to dedicate more effort to that in a separate PR. It would be nice though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I always forget how much generic types mess up the type data registrations haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah; on the other hand, they have a tendency to make them less useful as well, so 🤷
I wonder if we could generate an anonymous-scoped type that wraps the |
Not necessarily. Any fields marked |
That's fair, I was just thinking about
I was also wondering about stuff of this nature. Maybe worth investigating! |
Looks like the |
Merge conflicts are non-trivial; clean those up and I'll press the button. |
…ine#15493) (Note: bevyengine#15434 implements something very similar to this for functional curve adaptors, which is why they aren't present in this PR.) # Objective Previously, there was basically no chance that the explicitly-interpolating sample curve structs from the `Curve` API would actually be `Reflect`. The reason for this is functional programming: the structs contain an explicit interpolation `I: Fn(&T, &T, f32) -> T` which, under typical circumstances, will never be `Reflect`, which prevents the derive from realistically succeeding. In fact, they won't be a lot of other things either, notably including both`Debug` and `TypePath`, which are also required for reflection to succeed. The goal of this PR is to weaken the implementations of reflection traits for these structs so that they can implement `Reflect` under reasonable circumstances. (Notably, they will still not be `FromReflect`, which is unavoidable.) ## Solution The function fields are marked as `#[reflect(ignore)]`, and the derive macro for `Reflect` has `FromReflect` disabled. (This is not fully optimal, but we don't presently have any kind of "read-only" attribute for these fields.) Additionally, these structs receive custom `Debug` and `TypePath` implementations that display the function's (unstable!) type name instead of its value or type path (respectively). In the case of `TypePath`, this is a bit janky, but the instability of `type_name` won't generally present an issue for generics, which would have to be registered manually in the type registry anyway, which is impossible because the function type parameters cannot be named. (And in general, the "blessed" route for such cases would generally involve manually monomorphizing the function parameter away, which also allows access to `FromReflect` etc. through very ordinary use of the derive macro.) ## Testing Tests in the new `bevy_math::curve::sample_curves` module guarantee that these are actually `Reflect` under reasonable circumstances. --- ## Future changes If and when function item types become `Default`, these types will need to receive custom `FromReflect` implementations that exploit it. Such a custom implementation would also be desirable if users start doing things like wrapping function items in `Default`/`FromReflect` wrappers that still implement a `Fn` trait. Additionally, if function types become nameable in user-space, the stance on `Debug`/`TypePath` may bear reexamination, since partial monomorphization through wrappers would make implementing reflect traits for function types potentially more viable. --------- Co-authored-by: Gino Valente <[email protected]>
(Note: #15434 implements something very similar to this for functional curve adaptors, which is why they aren't present in this PR.)
Objective
Previously, there was basically no chance that the explicitly-interpolating sample curve structs from the
Curve
API would actually beReflect
. The reason for this is functional programming: the structs contain an explicit interpolationI: Fn(&T, &T, f32) -> T
which, under typical circumstances, will never beReflect
, which prevents the derive from realistically succeeding. In fact, they won't be a lot of other things either, notably including bothDebug
andTypePath
, which are also required for reflection to succeed.The goal of this PR is to weaken the implementations of reflection traits for these structs so that they can implement
Reflect
under reasonable circumstances. (Notably, they will still not beFromReflect
, which is unavoidable.)Solution
The function fields are marked as
#[reflect(ignore)]
, and the derive macro forReflect
hasFromReflect
disabled. (This is not fully optimal, but we don't presently have any kind of "read-only" attribute for these fields.) Additionally, these structs receive customDebug
andTypePath
implementations that display the function's (unstable!) type name instead of its value or type path (respectively). In the case ofTypePath
, this is a bit janky, but the instability oftype_name
won't generally present an issue for generics, which would have to be registered manually in the type registry anyway, which is impossible because the function type parameters cannot be named.(And in general, the "blessed" route for such cases would generally involve manually monomorphizing the function parameter away, which also allows access to
FromReflect
etc. through very ordinary use of the derive macro.)Testing
Tests in the new
bevy_math::curve::sample_curves
module guarantee that these are actuallyReflect
under reasonable circumstances.Future changes
If and when function item types become
Default
, these types will need to receive customFromReflect
implementations that exploit it. Such a custom implementation would also be desirable if users start doing things like wrapping function items inDefault
/FromReflect
wrappers that still implement aFn
trait.Additionally, if function types become nameable in user-space, the stance on
Debug
/TypePath
may bear reexamination, since partial monomorphization through wrappers would make implementing reflect traits for function types potentially more viable.